-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bump Zeebe to version 8.0.0-rc1 #264
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a hard one. The changes look ok. But now the behavior of InMemoryEngine differs from Zeebe. In Zeebe the consistency checks are governed by feature flags. Currently the checks are disabled by default.
The tricky bit is that we don't have any notion of feature flags yet in the InMemoryEngine. And maybe we should not? I am torn on this one, to be honest.
I also wonder whether the foreign key checks are in place because they happen on another level, or whether we have to reimplement those as well.
I see two ways going forward:
- we allow to pass in a configuration object and reimplement the relevant feature flags here
- we purposefully deviate from the behavior of Zeebe, but then I would at least try to mimick the default configuration, which currently is with integrity checks disabled.
There should also be tests in Zeebe for behavior of this class with feature flags enabled vs disabled. Maybe we should copy those as well.
After some discussion we have decided to keep the changes as they are at this moment. This means we will check for existing keys where relevant, and we will not check for foreign keys. The reasoning behind this decision is that the For this reason we have decided to deviate from the Zeebe database. We aim to keep the databases as similar as possible, but this is not always a feasible option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
two minor suggestions
...e/src/test/java/io/camunda/zeebe/process/test/engine/db/InMemoryZeebeDbColumnFamilyTest.java
Outdated
Show resolved
Hide resolved
...e/src/test/java/io/camunda/zeebe/process/test/engine/db/InMemoryZeebeDbColumnFamilyTest.java
Outdated
Show resolved
Hide resolved
The ColumnFamily interface in Zeebe has been modified. Before it only contained a put method. Starting from version 8.0.0 this has been replaced with update, insert and upsert methods. The delete method has been replaced with deleteExisting and deleteIfExists. In our in memory database we implement this interface. When updating to Zeebe 8.0.0 these changed need to be merged to keep the code from breaking.
The api changes to ColumnFamily are first available in Zeebe 8.0.0-rc1. We need to use this version to create our own release candidate. Upon upgrading I found there was another breaking change made in zeebe-protocol-jackson. For this reason a small change has been made to the way we do the mapping of records in ContainerizedEngine.
26ec1e8
to
f79df18
Compare
Description
The ColumnFamily interface in Zeebe has been modified. Before it only contained a put method. Starting from version 8.0.0-rc1 this has been replaced with update, insert and upsert methods.
The delete method has been replaced with deleteExisting and deleteIfExists.
In our in memory database we implement this interface. When updating to Zeebe 8.0.0-rc1 these changed need to be merged to keep the code from breaking.
In addition to the ColumnFamily changes the zeebe-protocol-jackson changed in the way we need to do the mapping. This minor mapping change has also been fixed in this PR.
Related issues
closes #262
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
Testing:
Documentation: